Skip to content

feat(container-runner): sleep on startup idle timeout - #5586

Open
abcxff wants to merge 1 commit into
stack/chore-container-runner-tighten-comments-zrxxyxlsfrom
stack/feat-container-runner-sleep-on-startup-idle-timeout-ontuyypk
Open

feat(container-runner): sleep on startup idle timeout#5586
abcxff wants to merge 1 commit into
stack/chore-container-runner-tighten-comments-zrxxyxlsfrom
stack/feat-container-runner-sleep-on-startup-idle-timeout-ontuyypk

Conversation

@abcxff

@abcxff abcxff commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review: feat(container-runner): sleep on startup idle timeout

Overall the change is well-scoped and the atomic one-shot idle_state flag plus abort_signal-cancelled timer task is a clean way to arm/disarm without extra locking. One correctness issue stands out; the rest are minor.

Correctness: on_sleep branches on the wrong signal

async fn on_sleep(self: Arc<Self>, ctx: Ctx<Self>) -> Result<()> {
    if idle_timeout().is_some() {
        self.stop_child(ctx.actor_id(), "actor sleeping (idle)").await;
    } else {
        self.drain_then_stop_child(ctx.actor_id(), "actor sleeping").await;
    }
    Ok(())
}

idle_timeout().is_some() only tells you the feature is enabled (RIVET_IDLE_TIMEOUT_SECS set), not that this particular sleep is the one-shot startup-idle sleep. The on_sleep doc comment notes the engine can sleep an actor for other reasons too (dashboard, crash policy, eviction), and those can happen at any point in a long-running, busy game session, not just at cold start.

With this code, once an operator turns the feature on, every sleep of that actor for its whole life skips the drain and force-stops the child within effective_stop_grace() (~9s) instead of waiting up to drain_grace() (15 min default) for it to exit on its own. That defeats the exact protection drain_then_stop_child exists for. A dashboard-triggered or eviction sleep mid-match would now kill the child abruptly whenever RIVET_IDLE_TIMEOUT_SECS happens to be configured, even though nothing about that particular sleep was actually idle-triggered.

Since self.idle_state already tracks whether this generation has ever received a request, checking that instead would fix it:

if self.idle_state.load(Ordering::Relaxed) == IDLE_ARMED {
    self.stop_child(ctx.actor_id(), "actor sleeping (idle)").await;
} else {
    self.drain_then_stop_child(ctx.actor_id(), "actor sleeping").await;
}

That only skips the drain when no request has ever arrived this generation (nothing to drain anyway), and preserves the full drain grace for every other sleep, matching the pre-existing behavior once the actor has actually been used.

Minor: TOCTOU between the idle check and ctx.sleep()

if this.idle_state.load(Ordering::Relaxed) == IDLE_REQUESTED {
    return;
}
...
if let Err(err) = ctx.sleep() { ... }

A request that arrives between the load and the ctx.sleep() call sets IDLE_REQUESTED too late; the timer still puts the actor to sleep while that request is being proxied to the child. This window is narrow and likely rare in practice, but combined with the on_sleep issue above (no drain), an unlucky in-flight request connection could get cut when the child is killed. If the on_sleep fix above lands, this race becomes much lower-impact since a genuinely in-flight request would still get the drain window. Not blocking, just worth being aware of.

Minor: undocumented env var

RIVET_IDLE_TIMEOUT_SECS is not mentioned in container-runner/README.md (note: neither are the sibling RIVET_DRAIN_GRACE_SECS / RIVET_SIGTERM_BUDGET_SECS knobs, so this is a pre-existing gap rather than a regression). Worth a short mention for operator discoverability while in the area.

Test coverage

No tests were added for the new behavior (timer arms/disarms correctly on first request, skip-drain vs. drain branching). container-runner has thin existing coverage (tests/inline/*) and none of it exercises actor.rs lifecycle logic yet, so this is not a new gap introduced by this PR, but an integration case driving RIVET_IDLE_TIMEOUT_SECS end-to-end (actor that never gets a request sleeps after the timeout; actor that does get a request does not) would help guard the behavior and would have caught the on_sleep issue above.

Style

Otherwise follows repo conventions well: hard tabs, structured tracing fields, complete-sentence comments, no em dashes, and Relaxed ordering is appropriate for this simple one-shot flag.

@abcxff
abcxff force-pushed the stack/feat-container-runner-sleep-on-startup-idle-timeout-ontuyypk branch from aee8b0b to ef4d957 Compare August 24, 2026 14:46
@abcxff
abcxff force-pushed the stack/chore-container-runner-tighten-comments-zrxxyxls branch from 102498a to 2e9fde1 Compare August 24, 2026 14:46
@abcxff
abcxff force-pushed the stack/feat-container-runner-sleep-on-startup-idle-timeout-ontuyypk branch from ef4d957 to d82ea5e Compare August 25, 2026 19:36
@abcxff
abcxff force-pushed the stack/chore-container-runner-tighten-comments-zrxxyxls branch from 2e9fde1 to 5b11c04 Compare August 25, 2026 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant